Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add $LESSOPEN and $LESSCLOSE support #2444

Merged
merged 5 commits into from
Sep 2, 2023
Merged

Add $LESSOPEN and $LESSCLOSE support #2444

merged 5 commits into from
Sep 2, 2023

Conversation

Anomalocaridid
Copy link
Contributor

@Anomalocaridid Anomalocaridid commented Jan 11, 2023 •
edited
Loading

Addresses #1739 and offers an alternative to #1597.

This pull request adds support for $LESSOPEN and $LESSCLOSE to bat. I tried to be as compatible with less's implementation as I could, and I tried to make sure it could handle each of the cases outlined in the "Input Preprocessor' section of less's man page. These cases are:

  • $LESSOPEN writing output to a temporary file that gets cleaned up by $LESSCLOSE
  • $LESSOPEN directly piping data and using empty output to signal to fall back to the original file
  • $LESSOPEN directly piping data and using the exit code to determine whether or not to fall back to the original file
  • The previous two cases, but also with $LESSCLOSE
  • All of the above cases, but reading from stdin instead of a file

Basically, the $LESSOPEN preprocessor is used to create an OpenedInput, falling back to the default file opening method if unable to preprocess the file. A type called Preprocessed, which implements BufRead, is used to wrap either a temporary file or data directly output by $LESSOPEN and used to create an InputReader. When $LESSCLOSE is set, it is executed when the Preprocessed is dropped.

Rather than just using std::process::Command, I used a crate called run_script, which runs shell commands in the shell rather than running a program and passing arguments directly, so that users can use things like input and output redirection directly like with less.

I also added a --no-lessopen flag similar to what less has, although I obviously could not add the corresponding -L flag since it is already in use. I updated the completion scripts where applicable.

In addition, I made sure to be as thorough as I reasonably could with the regression tests. However, I disabled the integration tests on Windows and other non-Unix-like systems with #[cfg(unix)] because they were not written with Windows in mind. Is this something that I should try to fix first?

I referenced #1597 to help me figure out an approach, although my implementation is still rather different and does not use any unsafe code.

Enselic
Enselic reviewed Mar 15, 2023
View reviewed changes
Copy link
Collaborator

@Enselic Enselic left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a quick skim-through of the code and to me this looks very good in general. The size of the PR makes it a bit hard to find a time slot and the energy to do a detailed review. It is not obvious to me that this PR can be split up into smaller chunks to mitigate that, however.

Just wanted to share some encouraging words. I know this PR have been sitting here for quite a while now. Hopefully someone will find the time to look into this PR in more detail in not a too distant future.

@Anomalocaridid
Copy link
Contributor Author

I understand. Thanks for letting me know.

@Anomalocaridid
Copy link
Contributor Author

Is there anything at all I can try to make my PR less cumbersome to review? I understand you all are busy and I would like to do whatever I can to make reviewing this pull request less of a burden.

Enselic
Enselic approved these changes Sep 1, 2023
View reviewed changes
Copy link
Collaborator

@Enselic Enselic left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should merge this and see what happens. If there are bugs, they can be fixed. Absolute worst case we have to revert this again, but just looking at the way you write code, tests, comments, that would be very surprising to me.

I don't have the time or energy to do a detailed review and understand this code. But I don't need to. If it works, it works. It always looks reasonable to me when I glance it over.

Can you rebase on latest master please?

@Anomalocaridid
Copy link
Contributor Author

Rebased. Thank both of you so much for taking another look at my pull request!

@Enselic Enselic merged commit e32ad0b into sharkdp:master Sep 2, 2023
21 checks passed
@sharkdp
Copy link
Owner

sharkdp commented Sep 2, 2023

Thank you very much for working on this!

@Anomalocaridid
Copy link
Contributor Author

It was my pleasure! Thank you too!

@Anomalocaridid Anomalocaridid deleted the lessopen branch September 2, 2023 16:26
@Aaron-Rumpler
Copy link

Maybe this should be opt-in instead of opt-out? Especially as this breaks spectacularly when $LESSOPEN is bat.

@Aaron-Rumpler
Copy link

Also, should bat be calling less with --no-lessopen by default? Right now, the file goes through $LESSOPEN twice (once by bat, and again by less).

@Anomalocaridid Anomalocaridid mentioned this pull request Sep 4, 2023
Merged
@Anomalocaridid
Copy link
Contributor Author

Maybe this should be opt-in instead of opt-out? Especially as this breaks spectacularly when $LESSOPEN is bat.

Good point. I failed to consider people that already use bat in $LESSOPEN.

Also, should bat be calling less with --no-lessopen by default? Right now, the file goes through $LESSOPEN twice (once by bat, and again by less).

Great catch! This helped significantly to figure out the cause of a bug in one of the tests that made it intermittently fail. It was somehow cause by less seeing $LESSOPEN.

Thanks for pointing these out. I'll make sure to address both of these concerns when I get the chance.

@Enselic Enselic mentioned this pull request Sep 5, 2023
Merged
@Enselic
Copy link
Collaborator

Enselic commented Sep 5, 2023

@Aaron-Rumpler Thank you for that crucial feedback 👍

@Anomalocaridid Thank you for taking care of these bugs!

Let's not enable lessopen by default (#2659) until we've had time to at least fix these known bugs. That way we are not blocked on making bat releases, and you @Anomalocaridid do not have any pressure to fix the bugs quickly.

@Enselic
Copy link
Collaborator

Enselic commented Sep 5, 2023

@Aaron-Rumpler Can you please confirm that the problems you reported do not manifest on latest git master now that the feature is not enabled by default? (#2659)

@Aaron-Rumpler
Copy link

Aaron-Rumpler commented Sep 6, 2023 •
edited
Loading

Yep, bat no longer uses $LESSOPEN by default on f49278c. bat still doesn't prevent less from using $LESSOPEN itself, but that's not new behaviour.

@Aaron-Rumpler
Copy link

Also, the man page still claims that $LESSOPEN is the default.

@Anomalocaridid
Copy link
Contributor Author

Also, the man page still claims that $LESSOPEN is the default.

I'll take care of that as well.

@Anomalocaridid Anomalocaridid mentioned this pull request Sep 6, 2023
Merged
@eth-p
Copy link
Collaborator

eth-p commented Dec 16, 2023

Oh this is awesome! Thanks for implementing this properly and taking the time to do it properly (unlike my original attempt).

@eth-p eth-p mentioned this pull request Feb 15, 2024
Open
@keith-hall keith-hall mentioned this pull request Mar 25, 2024
Closed
@akinomyoga akinomyoga mentioned this pull request Jun 9, 2024
Merged
@github-actions github-actions bot mentioned this pull request Jul 18, 2024
Merged
This was referenced Aug 16, 2024
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keith-hall keith-hall keith-hall approved these changes

@Enselic Enselic Enselic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Anomalocaridid @sharkdp @Aaron-Rumpler @Enselic @eth-p @keith-hall